Skip to content

test: Improve code coverage of tests#3063

Merged
miparnisari merged 19 commits into
authzed:mainfrom
josephschorr:test-cov-improvement
May 22, 2026
Merged

test: Improve code coverage of tests#3063
miparnisari merged 19 commits into
authzed:mainfrom
josephschorr:test-cov-improvement

Conversation

@josephschorr

Copy link
Copy Markdown
Member

Adds unit tests for various packages that had insufficient coverage

Covers every constructor, accessor, MarshalZerologObject, and
DetailsMetadata in the schema error types, plus asTypeError,
TypeError.Unwrap, NewTypeWithSourceError (with and without source
position), and backtickNames. Lifts pkg/schema to 80.7% coverage.
Covers every constructor, accessor, MarshalZerologObject, and
DetailsMetadata in the namespace error types.
Covers every constructor, Unwrap, GRPCStatus, MarshalZerologObject,
and accessor method in the graph error types.
Covers every constructor, GRPCStatus, and MarshalZerologObject in
the v1 services error types, plus the defaultIfZero helper.
Exercises full and minimal precondition/filter paths for
PreconditionFailedError and CouldNotTransactionallyDeleteError so
all branches in their GRPCStatus details are hit.
Adds tests for the previously-uncovered caveat, query,
reverse-query, count, and lookup-counter wrappers on the checking
replicated reader, the no-replicas degenerate path, and the
multi-replica round-robin selection via selectReplica.
Exercises the previously-uncovered caveat/namespace/count/lookup
wrappers on strictReadReplicatedReader, the replica-without-strict-
read-mode rejection path in NewStrictReplicatedDatastore, and the
primary-fallback branch in LegacyLookupNamespacesWithNames.
Adds error-path coverage for NewRelationshipIntegrityProxy (empty
and malformed key configs, duplicate key IDs, non-expiring expired
keys, current-key-with-expiration panic), full pass-through
coverage for the trivial datastore methods (MetricsID, UniqueID,
CheckRevision, Close, Features, OfflineFeatures, OptimizedRevision,
ReadyState, RevisionFromString, Statistics, Unwrap), reader
pass-throughs (CountRelationships, caveat/namespace reads,
LookupCounters), ReverseQueryRelationships hash-validation
failure, and BulkLoad both in the happy path and with a
pre-hashed relationship that triggers the bug-assertion panic.
Adds tests for the previously-uncovered public walker wrappers:
WalkCaveat, WalkCaveatWithOptions, WalkBaseRelation,
WalkBaseRelationWithOptions, WalkFlattenedSchema, the value-
receiver WalkOptions.WithStrategy, and WalkOptionsBuilder.MustBuild
on an errored builder. Also covers FlattenSchemaWithOptions with
nil input.
Adds direct unit tests for the two previously-uncovered public
functions in pkg/query: the NewSchemaArrow constructor (asserts it
marks the arrow as a schema arrow and configures left/right and
direction) and DatastoreIterator.ReplaceSubiterators (asserts it
panics since the datastore iterator is a leaf).
The pkg/datalayer package previously had no test files; any coverage
came from downstream consumers. Adds direct tests that wrap an
in-memory datastore and exercise:

- defaultDataLayer pass-throughs (ReadyState, Features, Stats, etc.)
- countingDataLayer query/reverse-query counters on both the
  snapshot reader and the RW transaction
- countingDataLayer pass-throughs and DeleteRelationships
- Unary and Stream counting interceptors, with and without exporters
- NewReadOnlyDataLayer write rejection and reader pass-throughs
- ReadWriteTransaction methods: WriteSchema, LegacySchemaWriter and
  each legacy writer method, BulkLoad, RegisterCounter /
  StoreCounterValue / UnregisterCounter
Extends combined_test.go to exercise the previously-uncovered option
setters (RelationshipChunkCacheConfig, RemoteDispatchTimeout,
CaveatTypeSet, Cache, ConcurrencyLimits, GrpcDialOpts,
RelationshipChunkCache) and the upstream-dispatch branch of
NewDispatcher. Adds error-path tests for invalid secondary
upstream hedging delays (parse failure and non-positive value),
invalid secondary dispatch expressions, and missing upstream CA
certificate files.
@github-actions github-actions Bot added area/api v1 Affects the v1 API area/datastore Affects the storage system area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) area/dispatch Affects dispatching of requests labels Apr 21, 2026
@josephschorr josephschorr changed the title Improve code coverage of tests test: Improve code coverage of tests Apr 21, 2026
@codecov

codecov Bot commented Apr 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.96%. Comparing base (a3d56d3) to head (1524644).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3063      +/-   ##
==========================================
+ Coverage   75.21%   75.96%   +0.75%     
==========================================
  Files         566      566              
  Lines       64250    64236      -14     
==========================================
+ Hits        48320    48789     +469     
+ Misses      12258    11775     -483     
  Partials     3672     3672              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephschorr josephschorr marked this pull request as ready for review April 21, 2026 18:09
@josephschorr josephschorr requested a review from a team as a code owner April 21, 2026 18:09
@josephschorr josephschorr enabled auto-merge (rebase) April 21, 2026 18:38
tstirrat15
tstirrat15 previously approved these changes Apr 22, 2026

@tstirrat15 tstirrat15 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments, otherwise LGTM

Comment on lines +84 to +94
dispatcher, err := NewDispatcher(
MetricsEnabled(true),
PrometheusSubsystem("test_subsystem"),
DispatchChunkSize(50),
RelationshipChunkCacheConfig(cfg),
CaveatTypeSet(caveattypes.Default.TypeSet),
Cache(dispatchCache),
ConcurrencyLimits(graph.ConcurrencyLimits{Check: 10, LookupResources: 5}),
)
require.NoError(t, err)
require.NotNil(t, dispatcher)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this assert that the options were applied? Or are we just saying that they don't cause errors when provided as inputs?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment goes for every test in this file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed code to be able to assert on the final options

Comment thread internal/services/v1/errors_test.go Outdated
Comment on lines +82 to +86
err := func() PreconditionFailedError {
var target PreconditionFailedError
_ = errors.As(NewPreconditionFailedErr(precondition), &target)
return target
}()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary because there isn't a New* constructor for this error type?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewPreconditionFailedErr returns the error interface, but the test needs the concrete PreconditionFailedError value to call GRPCStatus() on it.

Updated.

Comment thread pkg/datalayer/datalayer_test.go Outdated
Comment on lines +37 to +41
_, err = WalkFlattenedSchema(flattened, visitor, struct{}{})
require.NoError(t, err)

require.NotEmpty(t, visitor.schemas)
require.GreaterOrEqual(t, len(visitor.definitions), 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just testing that the code doesn't error, more or less?

"github.com/authzed/spicedb/pkg/tuple"
)

func TestCheckingReplicatedWithNoReplicasReturnsPrimary(t *testing.T) {

@miparnisari miparnisari Apr 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a godoc in the prod code that says

// NewCheckingReplicatedDatastore (...)
// NOTE: Be *very* careful when using this function. It is not safe to use this function without
// knowledge of the layout of the underlying datastore and its replicas.
// the replicas *must* point to a *stable* instance of the datastore (not a load balancer).

is it possible to encode this knowledge in a test?

Similarly, for NewStrictReplicatedDatastore:

// NewStrictReplicatedDatastore (...)
// This is useful when the read pool points to a load balancer that can transparently handle the request.

can we encode that in a test?

(see #2525 (comment) for more context)

Comment thread internal/graph/errors_test.go Outdated

@miparnisari miparnisari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. Just a few comments. The suggestion on the additional test can be done in a follow up because i think it's not trivial.

@miparnisari miparnisari force-pushed the test-cov-improvement branch from 22c698e to f8fbf4e Compare May 22, 2026 17:01
@miparnisari miparnisari requested a review from tstirrat15 May 22, 2026 17:25
auto-merge was automatically disabled May 22, 2026 18:29

Rebase failed

@miparnisari miparnisari merged commit 9a71382 into authzed:main May 22, 2026
45 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators May 22, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/api v1 Affects the v1 API area/datastore Affects the storage system area/dispatch Affects dispatching of requests area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) Skip-Changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants